Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reflect an user's primary_email_address update on its contact_info list #6585

Merged
merged 4 commits into from
Oct 4, 2023
Merged

Reflect an user's primary_email_address update on its contact_info list #6585

merged 4 commits into from
Oct 4, 2023

Conversation

nicholaspcr
Copy link
Contributor

@nicholaspcr nicholaspcr commented Sep 29, 2023 •
edited
Loading

Summary

Didn't create a issue for it but its linked to the list of behaviors noted while going through some of the contact_info changes.

References the discussion in #6515 (comment)

Changes in the interactions between the update of an user primary_email_address and its contact_info list.

There are a few areas of which the intended behavior was not happening:

  1. Effectively we do not update the primary email contact present in the contact_info list when an user performs a request to change its primary_email_address.
  2. When a non admin updates the primary_email_address, it does not set the primary_email_address_validated_at to nil. It also does not send a email re-requesting its validation.

Changes

  • Add primary_email_address_validated_at field mask when the update request is made by a non admin
  • Updates now changes both the primary_email_address and its value present in the contact_info list.
  • Add more unit test cases covering the sections of code changed

Testing

Unit tests and manual tests.

Test steps

Run the Stack with the following configuration:

tls:
  source: file
  root-ca: ./ca.pem
  certificate: ./cert.pem
  key: ./key.pem

is:
  email:
    dir: ".env/emails" # Example of directory path
    provider: "dir"
    sender-address: ""
    sender-name:  "[email protected]" # Example "[email protected]"
    templates:
      directory: # Example "<repo_path>/tts/pkg/email/templates"
  user-registration:
    contact-info-validation:
      required: true
      token-ttl: 48h0m0s

Non admin:

  1. Create a new non admin user ttn-lw-cli usr create <usr_id> --primary-email-address <email> --password <password>
  2. Validate the new user's email (should have a validation email on the path in dir on the config)
  3. Run tools/bin/mage dev:dbsql
    3.1. Run select primary_email_address, primary_email_address_validated_at from users;
    3.2. User with <usr_id> should be validated
    3.3. Run select * from contact_infos;
    3.4. Contact info with value should be validated.
  4. Login into the Console
  5. Access the <base_url>/oauth/profile-settings and update the Email Address
    5.1. Should receive a warning of restricted rights.
  6. Check the email folder, there should be a new validation email. This happens since the email is now not validated.
  7. Run tools/bin/mage dev:dbsql
    7.1. Run select primary_email_address, primary_email_address_validated_at from users;
    7.2. User with <usr_id> should be not be validated (validated_at being empty)
    7.3. Run select * from contact_infos;
    7.4. Contact info with value should not be validated. (validated_at being empty)

Admin:

  1. Create a new non admin user ttn-lw-cli usr create <usr_id> --primary-email-address <email> --password <password> --admin
  2. Validate the new user's email (should have a validation email on the path in dir on the config)
  3. Run tools/bin/mage dev:dbsql
    3.1. Run select primary_email_address, primary_email_address_validated_at from users;
    3.2. User with <usr_id> should be validated
    3.3. Run select * from contact_infos;
    3.4. Contact info with value should be validated.
  4. Login into the Console
  5. Access the <base_url>/oauth/profile-settings and update the Email Address
  6. Run tools/bin/mage dev:dbsql
    6.1. Run select primary_email_address, primary_email_address_validated_at from users;
    6.2. User with <usr_id> should be be validated
    6.3. Run select * from contact_infos;
    6.4. Contact info with value should be validated.
Regressions

Ever since 65edcaf when an user who updates its primary_email_address the operation is not reflected on the contact_info list. Therefore there is a disconnect between the primary email and the values present in the contact_info. The field has been deprecated, the primary email is what is used to send emails and we removed references to the contact_info field, being only kept for consistency.

Therefore I believe that this change is merely superficial and done in order to have the behavior as expected until the next major/bump and we can remove the contact_info field.

Notes for Reviewers

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • Documentation: Relevant documentation is added or updated.
  • The steps/process to test this feature are clearly explained including testing for regressions.
  • Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
  • Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

@nicholaspcr nicholaspcr added the technical debt Not necessarily broken, but could be done better/cleaner label Sep 29, 2023
@nicholaspcr nicholaspcr added this to the v3.28.0 milestone Sep 29, 2023
@nicholaspcr nicholaspcr self-assigned this Sep 29, 2023
@KrishnaIyer
Copy link
Member

@nicholaspcr: Please add the testing procedure.

@nicholaspcr nicholaspcr force-pushed the fix/user-update-email-validation branch from 3fb5f93 to b956d4a Compare October 3, 2023 10:25
@github-actions github-actions bot added the c/identity server This is related to the Identity Server label Oct 3, 2023
@nicholaspcr
Copy link
Contributor Author

The contact_info field even if deprecated, it being removed only on the major bump implies in the following scenario being possible in the user's update operation: an user updates primary_email_address and contact_info at the same time, if the new primary_email_address is not on the new contact_info values then there won't be an contact for the new primary_email_address value.

With the new changes done this should not longer be the case so I'm removing the text from the note for reviewers.

adriansmares
adriansmares reviewed Oct 3, 2023
View reviewed changes
pkg/identityserver/user_registry.go Outdated Show resolved Hide resolved
pkg/identityserver/user_registry.go Outdated Show resolved Hide resolved
@nicholaspcr nicholaspcr force-pushed the fix/user-update-email-validation branch from b956d4a to 38ae9d4 Compare October 3, 2023 13:13
@nicholaspcr nicholaspcr force-pushed the fix/user-update-email-validation branch from 38ae9d4 to b0e5e0f Compare October 3, 2023 14:08
@nicholaspcr nicholaspcr force-pushed the fix/user-update-email-validation branch from b0e5e0f to 884b4cd Compare October 3, 2023 16:35
@nicholaspcr nicholaspcr merged commit 894c503 into v3.28 Oct 4, 2023
@nicholaspcr nicholaspcr deleted the fix/user-update-email-validation branch October 4, 2023 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adriansmares adriansmares adriansmares approved these changes

Assignees

@nicholaspcr nicholaspcr

Labels
c/identity server This is related to the Identity Server technical debt Not necessarily broken, but could be done better/cleaner
Projects
None yet
Milestone
v3.28.0
Development

Successfully merging this pull request may close these issues.
3 participants
@nicholaspcr @KrishnaIyer @adriansmares